LGTM with nits

Two real bugs, correctly root-caused, with good test coverage. The parent-relative geometry fix is particularly well done — the before/after analysis in the PR description makes the failure mode crystal clear.

Findings

motionContextToDocs.ts — regex key injection (nit, low risk)
arrayAfterKey and scalarAfterKey interpolate key into new RegExp(...) without escaping. Currently safe (keys are \w+ property names from the snippet regex), but a stray metacharacter in a future key would silently misparse. Worth a one-line escape or a comment noting the invariant.

verify-motion.mjs:80 — shell injection via execSync (nit, low risk)

const err = execSync(`ffmpeg -i ${JSON.stringify(a)} -i ${JSON.stringify(b)} ...`).toString();

JSON.stringify isn't shell escaping — filenames with $() or backticks would be interpreted. Since this is an agent-facing tool (not user-facing), practical risk is low, but execFileSync with an array (already used elsewhere in the file) would close it.

Summary

• nodeToHtml parent-relative fix: correct, well-tested — boxOf(node) ?? parentBox cascading is the right pattern for CSS absolute positioning semantics
• CLI flag rejection skip for command groups: correct, three-case test matrix covers leaf-rejects / group-delegates / group-not-delegating
• motionContextToDocs: solid mechanical parser, good wrap-stripping logic, tests cover the interesting edge cases
• verify-motion.mjs: useful calibration tool, minor hardening opportunity

Ship it.

— Miga

Spot-check matches Miga's code read: packages/core/src/figma/nodeToHtml.ts:194 now uses the parent box for absolute child geometry, which matches CSS positioning semantics, and packages/cli/src/utils/command-failure-tracking.ts:40 skips group-level flag validation only when the first positional delegates to a known subcommand. The targeted tests cover both regression shapes.

Not stamping yet for procedural state: GitHub reports mergeable_state=dirty / mergeable=false, and the head currently only has WIP + Mintlify status checks. There is no trustworthy full CI result to approve through until the branch is rebased/conflicts are resolved and CI reruns. Miga's two nits on motionContextToDocs.ts:79 regex key interpolation and skills/figma/scripts/verify-motion.mjs:75 execSync shelling are valid but non-blocking from my read.

Verdict: COMMENT
Reasoning: Code shape looks approval-quality in the diff, but I can't stamp a dirty branch with no full CI run on the head.

— Magi

Reviewed at f883064.

Two well-scoped bug fixes, both correctly root-caused, plus a handy pair of infrastructure adds (motionContextToDocs, verify-motion.mjs) that materialise the field notes from the SDS brand-loop walkthrough into reusable helpers. PR body's before/after narrative is exemplary — I could reproduce the parent-relative reasoning without opening a single file, then verify it in the diff and it matched. Nothing 🔴 from my read; one 🟠 worth surfacing on the CLI fix, plus a couple of nits already landed on by Miga.

Layering on Miga's LGTM + nits and Magi's COMMENT-not-approve (procedural: mergeable_state=dirty on head f883064, only Mintlify + WIP checks on the SHA in statusCheckRollup — the earlier SHA 8530f6b did get a full CI green (CI / CodeQL / Windows render / preview-regression / Player perf / regression, all success, pull_request event), but that green is stale for the current head).

Core geometry (parent-relative figma children)

🟢 Correctness

nodeToHtml.ts:194-217 — the fix is right. boxOf(node) ?? parentBox cascading in renderChildren at :296 is the crucial piece: each depth's child subtracts its immediate positioned ancestor's absolute box, matching CSS position: absolute's "nearest positioned ancestor" semantics. The ?? parentBox fallback handles intermediate nodes with no bounding box correctly — since those nodes emit a <div> with no position set, CSS resolves the grandchild's absolute position against the next positioned ancestor up, and passing the grandparent's box down mirrors that. Root's origin default ({x:0,y:0,width:0,height:0} when the root has no box) means depth-1 children under a boxless root render at absolute canvas coords, which is the only sensible fallback.

Regression test at nodeToHtml.test.ts:52-89 asserts the specific integer values (left: 20px, top: 40px) AND explicitly rejects the double-offset shape (.not.toContain("left: 520px")), which is exactly the anti-test I want for a geometry fix — it can't accidentally pass a fixture regeneration.

🟢 Bonus fix (undocumented in PR body)

uniqueSlug at nodeToHtml.ts:164-176 — the digit-leading-slug prefix (n + slug when the slugified name starts with 0-9) is a real fix that's easy to miss in a diff scan. Test at :87-100 covers it. Worth calling out in the PR body next time — anyone rebasing on top of this and slugifying figma names elsewhere would want to know the invariant.

🟡 Nits (both already flagged by Miga)

• motionContextToDocs.ts:79-80,85 — arrayAfterKey / scalarAfterKey interpolate key into new RegExp(...) unescaped. Safe today because all callers pass \w+ property names (times, duration, ease, or property names harvested by /(\w+)\s*:\s*\[/g), but a one-line comment noting the invariant, or a RegExp.escape-shaped helper, would prevent a future caller from tripping it. Miga's suggestion.
• motionContextToDocs.ts balancedBlock doesn't skip over string bodies — a } inside a JSX string prop value would break brace-depth tracking. motion.dev's output is well-behaved (bezier arrays, named eases, no arbitrary strings), so this is a "future-proofing" note more than a bug. Fine as-is.

CLI groups (subcommand-flag rejection)

🟢 Correctness of the delegation-detection

command-failure-tracking.ts:40-47 — the shape is right. Group with subCommands set + a first-positional token that names a known subcommand ⇒ skip group-level assertKnownFlags. Three-case test matrix (leaf rejects, group delegates, group not delegating still rejects) covers the intent well.

🟠 Real concern — subcommand-level flag typos now silently ignored

Tracing this end-to-end: only the top-level commandLoaders in cli.ts:154-159 are wrapped by trackCommandFailures. The figma subcommand loaders defined in commands/figma.ts:52-57 (asset, tokens, component) are NOT wrapped. Under the OLD code, if you ran hyperframes figma component KEY:1-2 --bogus-flag, citty silently ignored --bogus-flag at the component leaf (per the whole reason reject-unknown-flags.ts exists), then the group-level wrapper caught it via its own assertKnownFlags — a false-positive on real flags (--name, per the PR body) but a true-positive on typos.

The fix removes the false-positive by skipping the group check on delegation. But it also removes the incidental true-positive protection at the leaf: hyperframes figma component KEY:1-2 --bogus now runs to completion with --bogus silently dropped. That's the exact class of silent-wrong-result that assertKnownFlags was written to prevent (see the docstring at reject-unknown-flags.ts:3-7) — the PR reintroduces it for every figma subcommand.

Practical example that would now silently succeed: hyperframes figma component KEY:1-2 --namee hero (typo of --name). Under the old code the group wrapper would have errored Unknown flag: --namee, giving you the typo hint. Under the new code, the import runs with the default component name — same class of "silent wrong result" the reject module set out to catch.

Suggested follow-up (doesn't have to block this PR — I'd take a TODO(vi): comment + a follow-up ticket): apply the same tracker wrapping inside commands/figma.ts for each subcommand loader. Roughly:

subCommands: {
  asset: trackCommandFailures(
    () => import("./figma/asset.js").then((m) => m.default),
    (err) => reportCommandFailure("figma asset", err),
  ),
  // ...
}

Or lift the wrapping into a small wrapSubCommands(name, loaders) helper that both cli.ts and each group can use.

If you already thought about this and decided the leaf-level typo protection isn't worth the wiring cost, a one-line note in the block-comment at :33-39 documenting that decision would save the next reader (or future you) a repeat trace.

🟡 First-positional heuristic edge case

command-failure-tracking.ts:42 — firstPositional = rawArgs.find((tok) => tok && !tok.startsWith("-")) treats any non-dash token as a positional. If a group ever grows a boolean flag whose value token happens to match a subcommand name (figma --level component KEY:1-2 --name x where --level is bool + component is its value in the shell's read order), the heuristic incorrectly infers delegation and skips validation. No group has flags today, so this is dormant, but a one-line comment noting the invariant ("this heuristic is sound only while command groups declare no flags of their own; if a group grows a flag, use a proper argv parse here") would keep the next contributor from silently invalidating it. Same nit shape as the regex-key one — worth surfacing to make the invariant load-bearing rather than incidental.

motionContextToDocs.ts + verify-motion.mjs

Both are additive infrastructure (no risk to existing paths) and both are well-shaped:

• motionContextToDocs.ts — the WRAP_EPSILON_S = 0.005 heuristic is defensible with the calibration in the header comment (SDS "Unlocked" card), tests exercise the four interesting shapes (single wrap marker, multi-keyframe wrap cluster, hold segment, multi-property node), and preserving bezier eases verbatim is right. The selectorFor callback pattern correctly forces the caller to supply Phase-3-import ids (a nice API affordance — the alternative of deriving from nodeName would silently drift).

• verify-motion.mjs — motion-energy delta comparison is a smart way to isolate choreography from static-fidelity noise. Calibration numbers in the header comment (faithful ≈ 20+dB, diverging ≈ 5dB, threshold 15) give the reader something concrete to reason about. Miga's execSync → execFileSync swap at :80 is a legitimate hardening opportunity (JSON.stringify isn't shell escaping — $() and backticks in a filename would be interpreted). Paths are all mkdtempSync-generated in practice so today's risk is zero, but the rest of the file already uses execFileSync with array args — trivial to align this one call.

Doc / manifest

skills/figma/SKILL.md:88-90 — updates the phase-2 write-up to point at the new mechanical helper and the verify script, both accurately. skills-manifest.json hash bump is consistent with the added file. LGTM.

Bundling

Both fixes surfaced in the same brand-loop walkthrough and touch the figma-mapper lane end-to-end (component import → geometry → motion translation → verification). Coherent as one PR — I wouldn't push to split.

What I didn't verify

• Whether the digit-leading-slug prefix (n prefix) collides with any existing DOM id convention or authored HTML in other imported components — unlikely, but not exhaustively grepped.
• Whether parseNodeTracks's inner while ((m = propRe.exec(animate)) !== null) at motionContextToDocs.ts:177 handles a motion.dev output where a property name repeats in a nested transition block (I don't think motion.dev emits that shape, but the regex is naive w.r.t. block structure — as noted, real-world safe).
• No local test-suite run — trusted the PR body's 96/96 core + 431/431 cli numbers plus the earlier-SHA CI green.

— Review by Rames D Jusso

Re-review at 666b84d7.

The prior procedural blocker is gone: the branch is rebased/mergeable, and the current head has full CI running with no red checks observed. Core checks including Build, Typecheck, Test, CLI smoke, Lint/Format, Skills manifest, Preview parity, and perf gates are green at review time; long regression shards / Windows / JS analysis are still pending and left to branch protection.

The substantive prior findings are addressed:

• Parent-relative Figma geometry is still the right fix: geometryCss subtracts the immediate parent box for non-root nodes (packages/core/src/figma/nodeToHtml.ts:194), and children pass boxOf(node) ?? parentBox downward (packages/core/src/figma/nodeToHtml.ts:296). The regression test pins the exact nested offsets and anti-asserts the old root-relative double-offset (packages/core/src/figma/nodeToHtml.test.ts:55).
• The CLI flag concern is fixed structurally. wrapCommand now recursively wraps subcommands (packages/cli/src/utils/command-failure-tracking.ts:29), while the group-level skip applies only when the first positional delegates to a known subcommand (packages/cli/src/utils/command-failure-tracking.ts:58). That preserves leaf typo protection. I verified this directly against the CLI source after building core: figma component KEY:1-2 --namee hero now errors Unknown flag: --namee, while --name hero passes flag validation and only fails on missing FIGMA_TOKEN.
• Miga's regex nit is fixed with escapeRegExp and both dynamic regex sites now use it (packages/core/src/figma/motionContextToDocs.ts:61, packages/core/src/figma/motionContextToDocs.ts:88, packages/core/src/figma/motionContextToDocs.ts:104). The balanced-block string-literal invariant is documented (packages/core/src/figma/motionContextToDocs.ts:67).
• Miga's shelling nit is fixed: the PSNR probe now uses spawnSync("ffmpeg", [...]), no shell interpolation (skills/figma/scripts/verify-motion.mjs:110).

Local verification: command-failure-tracking.test.ts 11/11, nodeToHtml.test.ts + motionContextToDocs.test.ts 21/21, plus the direct CLI typo/valid-flag smoke described above.

No remaining blockers from my prior review.

— Magi

Verdict: APPROVE
Reasoning: The branch now has the full CI surface running, the earlier dirty-branch gate is resolved, and the CLI leaf-flag regression plus both nits have been fixed and verified at source and behavior level.